Harden fetchAndTestRpcs parsing against DefiLlama format changes and add RPC fallbacks#638
Conversation
Deploying goodprotocolui with
|
| Latest commit: |
996a90d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://49642df0.goodprotocolui.pages.dev |
| Branch Preview URL: | https://copilot-fix-fetch-and-test-r.goodprotocolui.pages.dev |
fetchAndTestRpcs parsing against DefiLlama format changes and add RPC fallbacks
…se reset on failure
|
Handoff — ready for review @L03TJ3 What changed from the Copilot PRThe original implementation fetched a raw
In What was tested
Remaining risks
|
|
@Gutopro why are you enforcing pnpm, the package manager for the repository is yarn? |
|
the test all pass now @L03TJ3 |
- extend test with fallback test case
|
going to approve but please follow up on my telegram message @Gutopro |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The fallback balance logic in
Web3StatusInnermixesnativeBalance(likely already formatted) withdirectNativeBalance(a rawgetBalancevalue in wei), but both are run throughparseFloat(...).toFixed(4); consider normalizing units so the direct RPC balance is converted with the correct decimals before display to avoid misleading amounts. - In
Web3StatusInner,useEthersis cast toanyandlibraryis assumed to havegetBalance; it would be more robust to type this hook properly (or narrow the type) so thatlibrarycan be safely used and refactoring is less error-prone. - The Jest setup introduces
jest29.x whilejest-environment-jsdomis at 30.x; aligning these versions (or dropping the explicitjest-environment-jsdomdevDependency and relying on the built-in environment) will reduce the risk of subtle test runner incompatibilities.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The fallback balance logic in `Web3StatusInner` mixes `nativeBalance` (likely already formatted) with `directNativeBalance` (a raw `getBalance` value in wei), but both are run through `parseFloat(...).toFixed(4)`; consider normalizing units so the direct RPC balance is converted with the correct decimals before display to avoid misleading amounts.
- In `Web3StatusInner`, `useEthers` is cast to `any` and `library` is assumed to have `getBalance`; it would be more robust to type this hook properly (or narrow the type) so that `library` can be safely used and refactoring is less error-prone.
- The Jest setup introduces `jest` 29.x while `jest-environment-jsdom` is at 30.x; aligning these versions (or dropping the explicit `jest-environment-jsdom` devDependency and relying on the built-in environment) will reduce the risk of subtle test runner incompatibilities.
## Individual Comments
### Comment 1
<location path="src/components/Web3Status/index.tsx" line_range="74-78" />
<code_context>
+ }
+ }, [address, library, /*used*/ chainId])
+
+ const displayNativeBalance = nativeBalance ?? directNativeBalance
+ const shouldShowBalance = showBalance && !!displayNativeBalance
</code_context>
<issue_to_address>
**issue (bug_risk):** Direct balance fallback is in base units and will be rendered as a huge, mis-scaled value.
`nativeBalance` from `useNativeBalance` appears to be in human-readable units, but `directNativeBalance` is the raw `library.getBalance(address)` result (a wei `BigNumber` in ethers v5). Using it directly (or via `parseFloat`) will display wei as if it were ETH/FUSE, off by 10^18. Normalize `directNativeBalance` first (e.g. `ethers.utils.formatUnits(balance, decimals)`) so its units match `nativeBalance` before computing `displayNativeBalance`.
</issue_to_address>
### Comment 2
<location path="src/functions/rpcParsing.ts" line_range="20-21" />
<code_context>
+ const result: Record<string, string[]> = {}
+ for (const chain of chains) {
+ if (TARGET_CHAIN_IDS.has(chain.chainId)) {
+ result[String(chain.chainId)] = chain.rpc.filter(
+ (url) => url.startsWith('http://') || url.startsWith('https://')
+ )
+ }
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Including plain HTTP RPC URLs may introduce security/privacy risks.
This currently allows both `http://` and `https://` RPC endpoints. For public/production networks, `http://` risks traffic tampering and data leakage. Consider restricting to `https://` for mainnets, and only allow `http://` explicitly for local/test setups (e.g., behind a feature flag or env-based check).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const displayNativeBalance = nativeBalance ?? directNativeBalance | ||
| const shouldShowBalance = showBalance && !!displayNativeBalance | ||
|
|
||
| const sortedRecentTransactions = useMemo(() => { | ||
| const txs = Object.values(allTransactions) |
There was a problem hiding this comment.
issue (bug_risk): Direct balance fallback is in base units and will be rendered as a huge, mis-scaled value.
nativeBalance from useNativeBalance appears to be in human-readable units, but directNativeBalance is the raw library.getBalance(address) result (a wei BigNumber in ethers v5). Using it directly (or via parseFloat) will display wei as if it were ETH/FUSE, off by 10^18. Normalize directNativeBalance first (e.g. ethers.utils.formatUnits(balance, decimals)) so its units match nativeBalance before computing displayNativeBalance.
Fixes #637
fetchAndTestRpcswas failing due to brittle parsing of DefiLlama'sextraRpcs.jssource file, which could leave tested RPC sets empty. This update replaces the custom JS parser with a simple JSON fetch and guarantees fallback RPCs when any step fails.What changed
src/hooks/rpcParsing.ts— replaced ~150-line balanced-segment JS parser with afetchRpcsFromChainlist()function that fetcheschainid.network/chains.json(a stable JSON endpoint), filters for required chains (1,122,42220,50), and strips non-HTTP(S) URLs. No custom parsing, noeval(), no regex on JS source.src/hooks/useWeb3.tsx— updatedfetchAndTestRpcsto callfetchRpcsFromChainlist()directly, removed theCHAINLIST_URLconstant andchainMappingobject, and addedrpcInitializationPromise = nullin the catch block so the app can retry on failure without a page reload.src/hooks/rpcParsing.test.ts— updated to test the new API: live fetch againstchainid.networkasserting all 4 chains return valid HTTP(S) URLs, plus a unit test forFALLBACK_RPCS_BY_CHAIN.Test evidence
Full suite: 44 tests passing, 7 pre-existing failures unrelated to this PR (
@sushiswap/sdkpnpm hoisting,@reown/appkitESM — both present onmaster).pnpm lint— clean.pnpm build— fails with pre-existing@ethersproject/bignumberrollup resolution error (pnpm hoisting incompatibility, not introduced by this branch).Summary by Sourcery
Improve RPC discovery and network UX while hardening tests and CI around RPC parsing.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Tests: